You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When the driver materializes a UDT value, the type's assembly-qualified name comes from the server. That name previously flowed into Assembly.Load and Type.GetType without the driver making any decision about it, so the set of assemblies a connection could pull into the process was effectively chosen by the server rather than by the application.
This PR puts the application back in control with a deny-by-default policy, plus a validation gate on the resolved type.
Policy (UdtAssemblyPolicy)
A single enforcing behavior that permits exactly three things:
Permitted
Notes
Microsoft.SqlServer.Types
Identity pinned. Version is normalized to the connection's negotiated type system version and the public key token to the one Microsoft signs with, so the built-in exemption cannot be satisfied by a same-named assembly sitting on the probing path.
Assemblies on an application-supplied allow list
The app explicitly naming what it is willing to have loaded.
Assemblies already loaded into the process
Resolved to the instance the process already holds; the server-supplied version and public key token are discarded.
Everything else is refused. Notably, an assembly that is only statically referenced by a loaded assembly is not permitted, because loading it is a genuinely new load, which is precisely the decision this keeps with the application.
Applications configure the allow list through an AppContext data element:
Each entry matches only on the components it specifies, so a simple name permits any version/culture/PKT while a fully-qualified name must match exactly.
Type validation
Independently of the assembly decision, a resolved type that is not annotated with SqlUserDefinedTypeAttribute is now rejected before any of its code runs. This is the gate that actually prevents foreign code execution. I verified empirically on CoreCLR that neither Assembly.Load, nor resolving a type, nor reading that type's custom attributes runs anything from the target assembly. A module initializer or static constructor runs on first real member access, which is what GetUdtValue would otherwise perform. So the attribute check sits in front of the only step that executes code.
SmiMetaData.Type had a second, latent sink for the same pattern; it is now routed through the same policy as defense in depth, even though all live callers pass null today.
Escape hatch
Switch.Microsoft.Data.SqlClient.UseLegacyUdtAssemblyLoad restores the previous behavior in full. It is intended as a temporary compatibility measure, not a supported configuration, and is documented as such.
Backwards compatibility
This is a behavior change for applications using custom UDTs, and I did not find a way to avoid it without leaving the hole open. Flagging it prominently rather than burying it.
The built-in spatial types (SqlGeography, SqlGeometry, SqlHierarchyId) are unaffected.
An application is affected when the custom UDT's assembly is not already loaded at the moment the value is read. That happens whenever the driver materializes the value and the app never names the type in its own code: generic data access layers, micro-ORMs, DataTable.Load, and schema discovery. In those cases the driver's own Assembly.Load was the only thing pulling the assembly in. If the app statically names the UDT type, the JIT loads the assembly first and everything still works.
Two different symptom shapes, and the second is the one worth reviewer attention:
API
Symptom
reader[i], GetValue, UDT output parameters
SqlException naming the assembly and the allow list
GetFieldType, GetSchemaTable, GetColumnSchema
Returns null for the UDT column's type rather than throwing
The second row follows the pre-existing fThrow: false contract on those paths, so I preserved it rather than changing unrelated behavior in a security fix. It is harder to diagnose, because GetFieldType does not normally return null and a caller that dereferences the result sees an unrelated NullReferenceException. To compensate, every denial is traced through SqlClientEventSource regardless of which path was taken, so event source tracing will always identify the assembly. I would welcome a second opinion on whether that trade is right, or whether these paths should throw despite the contract.
The remedy in every case is to name the assembly on the allow list.
Documentation and localization
.github/instructions/features.instructions.md documents the switch, the permitted set, and a "Compatibility impact" section covering both symptom shapes.
Two new resource strings (SQLUDT_AssemblyNotAllowed, SQLUDT_TypeNotUserDefined) are added to Strings.resx. Localization will pick these up through the normal OneLocBuild flow after this merges.
No public API surface changes, so no ref/ updates are needed.
Issues
Tracked internally via the MSRC case and its linked ADO repair item. Intentionally not linking a public issue here while the case is under coordinated release.
Testing
Two new unit test files, 85 tests total across the policy and the switch:
UdtAssemblyPolicyTest.cs covers the policy in isolation: enforcement, the pinned Microsoft.SqlServer.Types identity (including rejecting a same-named assembly with the wrong PKT/version), deny-by-default, allow list matching at each level of qualification, and the already-loaded tier. Includes Resolve_LoadedAssembly_IgnoresServerSuppliedIdentity, which covers a bug found during self-review where a loaded assembly was matched on simple name but then loaded using the server's full reference, letting a server force a new load of a different version.
UdtAssemblyLoadHardeningTest.cs drives CheckGetExtendedUDTInfo end to end with hostile assembly-qualified names and asserts no load occurred. It also asserts the attribute gate rejects a non-UDT type without running its static constructor, reading the marker flag from a separate class so the assertion is meaningful.
Both test classes join AppContextSwitchTestCollection so they serialize with the other AppContext-mutating tests.
Also added coverage for a structural limitation worth knowing about: a bare type name with no assembly part never reaches the assembly resolver at all, so only the attribute gate stops it. Three regression tests pin that behavior.
Validation performed: clean build at 0 warnings under TreatWarningsAsErrors; full unit suite 992 passed / 9 skipped / 3 failed, where the 3 failures are pre-existing macOS keychain issues in NativeColumnEncryptionKeyBaseline unrelated to this change.
Gap: the net462 leg cannot be built on macOS, so it needs CI to validate. Related open question: I measured module-initializer timing only on CoreCLR. ECMA-335 permits a runtime to run module initializers at load time, and .NET Framework is unverified. A different result there would mean softening how the docs frame the ordering, but the design is safe either way since the attribute gate still runs before any member access.
Guidelines
Tests added
Public API changes documented (no public API surface change)
Ensure no breaking changes introduced - intentionally unchecked, see "Backwards compatibility" above
SqlConnection.ResolveTypeAssembly handed the assembly name carried by a
server-supplied UDT assembly-qualified name straight to Assembly.Load,
and GetUdtValue then invoked a static member on the resolved type
without checking that it was a user-defined type at all. Loading an
assembly runs its module initializer and invoking a static member runs
the type's static constructor, so a compromised or hostile server -- or
an attacker on the network path of a connection that has opted out of
certificate validation -- could choose which code the client process
executes.
Add a deny-by-default policy that decides whether an assembly may be
loaded before the name reaches the loader:
- Restricted (default) permits Microsoft.SqlServer.Types, the
application's allow list, assemblies already loaded into the process,
and assemblies statically referenced by them.
- Strict, via Switch.Microsoft.Data.SqlClient.UseStrictUdtAssemblyLoad,
drops the loaded/referenced allowance.
- Legacy, via Switch.Microsoft.Data.SqlClient.UseLegacyUdtAssemblyLoad,
restores the previous behavior as a compatibility escape hatch and
takes precedence over Strict.
The Microsoft.SqlServer.Types exemption now pins the public key token as
well as the version, so it cannot be satisfied by a same-named assembly
on the probing path.
Independently of the mode, CheckGetExtendedUDTInfo now rejects a
resolved type that is not annotated with SqlUserDefinedTypeAttribute.
Reading custom attributes does not run a static constructor, so this is
the last point at which the driver can decline without executing any of
the type's code, and it covers every call site uniformly.
Applications with lazily-loaded custom UDT assemblies can name them
through the Microsoft.Data.SqlClient.UdtAssemblyAllowList AppContext
data element.
The known-assembly-name set is cached and invalidated only by
AppDomain.AssemblyLoad, so a server streaming distinct names costs a
hash lookup rather than a probe.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
Fixes six issues found while reviewing the initial hardening commit:
- CheckGetExtendedUDTInfo now wraps SqlUdtInfo.TryGetFromType in a
catch, so attribute resolution failures do not start throwing at the
fThrow: false call sites (GetFieldType and the provider-specific field
type) that previously tolerated an unresolvable UDT.
- SmiMetaData.Type's assembly-qualified name fallback was a second,
ungated Type.GetType sink. It now resolves through the same policy.
Every reachable caller currently passes null, so this is defence in
depth rather than a live hole.
- Pinning the identity of Microsoft.SqlServer.Types is now folded into
UdtAssemblyPolicy.IsAllowed, so it is not possible to consult the
policy without also pinning. The built-in exemption is granted on the
simple name alone, so an unpinned reference would have let an unsigned
assembly borrow the name.
- The known-assembly-name set is now maintained incrementally by the
AssemblyLoad handler instead of being rebuilt on every load, removing
a full enumeration of the process's assemblies and their reference
lists from the hot path.
- Adds regression tests for a type name that carries no assembly part.
Type.GetType resolves such a name without ever consulting the assembly
resolver, so the SqlUserDefinedTypeAttribute check is the only gate it
passes through; the tests lock that in for both fThrow values.
- Verified that an exception thrown from inside the assembly resolver
propagates out of Type.GetType unwrapped for both throwOnError values,
so SQL.UdtAssemblyNotAllowed reaches the caller intact.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
The merge of origin/main brought in PR #4495, which introduced
AppContextSwitchTestCollection to serialize tests that mutate
process-wide cached AppContext switch values. Both UDT test classes do
exactly that, so they join the collection; without it they can race
against other collections and observe each other's temporary settings.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
Replaces the Restricted/Strict/Legacy taxonomy with one enforcing
behavior plus the legacy escape hatch, and removes the
UseStrictUdtAssemblyLoad switch.
The three-mode design was over-built for what this layer does. Measured
on CoreCLR, neither Assembly.Load, nor resolving a type from the loaded
assembly, nor reading that type's custom attributes executes any code
from the target; a module initializer runs on first real member access,
which is what GetUdtValue would perform. The SqlUserDefinedTypeAttribute
check in CheckGetExtendedUDTInfo is therefore the gate that actually
prevents foreign code execution, and the assembly policy in front of it
is a resource-load gate that does not warrant two tiers.
The single enforcing mode permits the pinned Microsoft.SqlServer.Types
assembly, the application's allow list, and assemblies already loaded
into the process. The static reference closure is no longer permitted,
because loading a referenced-but-unloaded assembly is a genuinely new
load, which is the thing this policy exists to keep under the
application's control. Applications with custom UDTs whose assembly is
not yet loaded must now name it on the allow list.
Also fixes an identity-binding hole in the already-loaded tier. It
matched on simple name and then handed the server's full reference,
including version and public key token, to the loader, so a server could
name a loaded simple name with a different identity and still trigger a
new load. The policy now returns the loaded instance itself, and
callers use it rather than re-binding server-controlled identity.
Note that ECMA-335 permits a runtime to run a module initializer at load
time, and only CoreCLR was measured here, so the assembly policy is
retained as defence in depth pending verification on .NET Framework.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
Records the decision to ship the single enforcing mode as-is and accept
the compatibility break rather than staging the reference closure behind
a deprecation release.
Documents which applications are affected, why the affected shape is
common (the driver materializes the value and the application never
names the UDT type itself, so the driver's own Assembly.Load was
previously what pulled the assembly in), and both symptom shapes. The
fThrow: false path is called out specifically, because GetFieldType,
GetSchemaTable and GetColumnSchema return null for a denied UDT column
rather than throwing, and a caller that dereferences the result sees an
unrelated NullReferenceException. Denials are always traced, so event
source tracing identifies the assembly in either case.
No behavior change.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
Implements assembly-load policy and allow-list matching.
Critical (2 votes): Post-load identity is not validated. Critical (1 vote): Culture remains server-controlled for the built-in identity. Critical (4 votes): Explicit PublicKeyToken=null is treated like an omitted token. Moderate (3 votes): Global strong references can prevent collectible assemblies from unloading.
The unconditional claim that the type is rejected before any code runs is not established for net462: the tests and description only measured CoreCLR, and the PR notes that .NET Framework module-initializer timing is unverified. Qualify this guarantee by runtime or state the portable guarantee in terms of no member access before the attribute check, so the security documentation does not promise an ordering that has not been verified.
Independently of the assembly policy, a resolved type that is not annotated with
`SqlUserDefinedTypeAttribute` is rejected before any of its code runs (except
under `UseLegacyUdtAssemblyLoad`). This is the gate that actually prevents
foreign code execution: on CoreCLR, neither `Assembly.Load`, nor resolving a type
from the assembly, nor reading that type's custom attributes runs anything from
.github/instructions/features.instructions.md:329
The new SmiMetaData.Type resolver throws SQL.UdtAssemblyNotAllowed directly and does not emit the SqlClientEventSource denial event used by ResolveTypeAssembly. Therefore a denial on this path is not traced despite the documentation claiming that every denial is identifiable through event-source tracing. Either add the same trace before throwing or narrow this statement to the paths that actually log the denial.
`NullReferenceException`. A denial is always traced through
`SqlClientEventSource` regardless of which path was taken, so enabling event
source tracing will identify the assembly.
This path applies the assembly policy but never applies the new SqlUserDefinedTypeAttribute gate. ValueUtilsSmi.NullUdtInstance invokes metaData.Type's static Null member, so a bare or permitted non-UDT name can still execute untrusted type code through SMI even though CheckGetExtendedUDTInfo blocks it in the main path. Validate the resolved type before storing or returning it, including names that bypass assemblyResolver.
The PR description says denied UDT materialization produces a SqlException, but this new helper constructs a TypeLoadException through ADP.TypeLoad (and the non-UDT type helper does the same). That is user-visible error behavior, so either the implementation needs to throw the promised exception type or the description and tests should be corrected to document the actual contract.
Because Contoso.Evil is intentionally nonexistent, the pre-fix implementation would still call Assembly.Load, receive FileNotFoundException, and raise no AssemblyLoad event. Thus these assertions pass even when the loader is reached and do not prove that the policy short-circuits the load. Use a real same-name test assembly/loader hook or assert a policy-specific failure/trace.
The fallback comment refers to Resolve_UnknownAssembly_IsDenied, but that test does not exist; the companion method is IsAllowed_UnknownAssembly_IsDenied above. Correct the reference so maintainers can locate the intended deny-path coverage.
// Resolve_UnknownAssembly_IsDenied covers the general deny path.
The reason will be displayed to describe this comment to others. Learn more.
I've added a few comments.
What are your thoughts about pressing #4413 forwards rather than using a static AppContext string? This would force clients to register the precise UDTs they're expecting from servers and it'd mandate that the types are already loaded (because someone has to call SqlUserDefinedTypeRegistration.Register<T> or similar with a static generic type parameter.) It also means that there's less machinery behind the allow-lists because we don't need to try to discover already-loaded assemblies.
The reason will be displayed to describe this comment to others. Learn more.
If foreign code execution is the primary concern then GetCustomAttributes will run the attributes' constructors and module initializers. CustomAttributeData may be more relevant for our use case.
The reason will be displayed to describe this comment to others. Learn more.
Worth checking, so I measured it. It does not apply here, for two reasons.
SqlUserDefinedTypeAttribute is sealed, so a hostile assembly cannot subclass it to get its constructor invoked through the filter. And SqlUdtInfo.TryGetFromType uses the filtered overload, GetCustomAttributes(typeof(SqlUserDefinedTypeAttribute), false), which does not instantiate non-matching attributes.
Probe on net9.0 with a hostile assembly carrying a module initializer, a non-matching attribute whose ctor writes a file, and a type with a static ctor:
Step
module init
non-matching attr ctor
type cctor
Assembly.LoadFrom
no
no
no
GetType
no
no
no
GetCustomAttributes(typeof(T), false)
no
no
no
CustomAttributeData.GetCustomAttributes
no
no
no
InvokeMember("Null")
yes
no
yes
So CustomAttributeData and the filtered overload are equivalent for this use, and I left the existing SqlUdtInfo call alone rather than change shared code in a security fix.
Your point does hold for the unfiltered overload. Since the safety here rests on two properties that are easy to lose by accident - the attribute being sealed, and the lookup being filtered - I have written both into the docs so a future change does not silently remove the guarantee.
The reason will be displayed to describe this comment to others. Learn more.
Another possibility might be to have a new public type, UnregisteredUserDefinedType, and document the circumstances in which it is returned.
If so, clients sometimes use Activator.CreateInstance on the type. In such cases, having the default ctor throw would be a reasonably simple point of contact for them.
The reason will be displayed to describe this comment to others. Learn more.
I like this, and it is a better answer than what the PR currently does for the silent-null path.
The appeal is that it removes the worst diagnostic problem here. Right now GetFieldType returning null on a denied UDT is nearly undiagnosable: a caller dereferences it and sees a NullReferenceException with no connection to the actual cause. A sentinel type keeps the non-throwing contract those call sites rely on while still carrying the explanation, and a throwing default constructor gives Activator.CreateInstance callers a precise point of contact, as you say.
Two things I would want to settle before doing it:
It is new public API surface, so it needs API review, and this PR is on an MSRC release schedule. I would rather not couple the two.
The sentinel would flow into GetSchemaTable().DataType and GetColumnSchema().SqlDataType, so we should decide deliberately what those should show. A type whose name states the problem is arguably an improvement over null, but it is a visible change to schema output either way.
My suggestion is to ship the deny-by-default boundary here and do UnregisteredUserDefinedType as a focused follow-up with proper API review, rather than rush a public type into a security fix. If you would rather it land together I am happy to add it - your call as maintainer.
The reason will be displayed to describe this comment to others. Learn more.
Thanks - I worked through these, and one of them does not behave the way you expected.
Not affected, for the reason you give (we move bytes without interpreting them):
SqlBulkCopy to a UDT column: SqlBulkCopy.cs:853 maps UDT to varbinary in the bulk command text.
SqlBulkCopy from a DataTable: values arrive as objects the application already holds.
SqlBulkCopy to varbinary(max): no type resolution.
Affected, contrary to expectation:
SqlBulkCopyfrom a SqlDataReader between UDT columns. SqlBulkCopy.cs:1241 calls _sqlDataReaderRowSource.GetValue(sourceOrdinal) so it can test the value for INullable. GetValue materializes the UDT, so it goes through the policy, and a denial surfaces as a TypeLoadException mid-copy.
Table-valued parameters sourced from a SqlDataReader. SqlParameter.cs:1295 calls GetInternalSmiMetaData, which hits SqlDataReader.cs:310 with fThrow: true.
So the "transferring bytes without interpretation" intuition holds everywhere except where we need INullable or SMI metadata, and in both of those we materialize the type.
SqlCommandBuilder I could not find a UDT type-resolution path in at all - it works from column metadata names rather than CLR types - so I believe it is unaffected, but I would value a second opinion since you raised it.
I will fold the two affected cases into the compatibility documentation.
The reason will be displayed to describe this comment to others. Learn more.
This appears to leave the scenario below:
Assembly A is trustworthy, Assembly B has already been loaded by the client (by some other means.)
We whitelist Assembly A and the server sends a UDT from it.
Assembly A loads. UdtAssemblyPolicy reads the list of currently-loaded assemblies and trusts them.
Server sends a UDT from Assembly B. The client hasn't trusted it - but because it was already loaded, a type from it is instantiated.
Is this deliberate?
At first glance, I was concerned about cases where Assembly B is a dependency of Assembly A and inherited its trust as a result of being loaded. This doesn't happen because we only build s_loadedAssemblies from GetAssemblies once, but it might be worth noting that this isn't just a performance optimisation - it's got security properties.
The reason will be displayed to describe this comment to others. Learn more.
Yes, deliberate - but you are right that it was under-documented, and I have now written it down rather than left it implicit.
The already-loaded tier makes the permitted set a property of the process, not of the connection. Once Assembly B is loaded by any means, a UDT type inside it can be instantiated on the say-so of any server the process talks to. The attribute gate confines this to types carrying SqlUserDefinedTypeAttribute - that is, types written to be deserialized from SQL Server - but it is a real widening and now says so explicitly.
The reasoning for keeping it: without it, the common case of an application that statically references its own UDT type breaks, because the driver's Assembly.Load was previously what pulled the assembly in. Removing the tier would turn a targeted hardening into a broad compatibility break. The narrower alternative is per-server or per-type trust, which is the thread on SqlUserDefinedTypeRegistration.
Your second paragraph is the sharper observation, and I am glad you flagged it. Building s_loadedAssemblies once is not just a performance optimisation - if it were rebuilt on demand, an assembly pulled in as a dependency of a permitted assembly would silently inherit that permission, which is exactly the transitive trust the policy is meant to prevent. That is now stated in the field's documentation as a property to preserve, so nobody later "fixes" it into a lazy refresh.
The reason will be displayed to describe this comment to others. Learn more.
Broader note for here and elsewhere: .NET uses AssemblyLoadContexts rather than AppDomains. We'd continue to want to use the current ALC/AppDomain rather than AssemblyLoadContext.Default - this'll break ALC unloadability.
I was using AppDomain.CurrentDomain.GetAssemblies() rather than AssemblyLoadContext.Default, but the effect you describe was still there via the strong references: a server could force the map to be built with a single denied UDT and thereby pin every assembly in the process, including collectible plugins.
Two changes: the map now holds WeakReference<Assembly> and drops dead entries on lookup, and the tier is scoped to the ALC that loaded the driver, which is where the driver's own Assembly.Load resolves anyway. Assemblies in other contexts were never reachable by name from here, so recording them was over-broad for trust and useless for resolution. An application that loads its UDT assembly into a separate collectible context now needs to allow-list it, which is documented.
Addresses review feedback on the UDT assembly load hardening.
The central issue: on .NET the loader ignores the public key token in an
AssemblyName, so pinning the reference was not an enforcement boundary. A
caller could consult the policy and still be handed a same-named assembly
with a different identity. SqlAuthenticationProviderManager already
documents this for the Azure extension assembly and compensates with a
post-load check; the UDT path now does the same.
To make that impossible to forget, the load moves inside the policy.
TryResolve (decide, then let the caller load) is replaced by TryLoad,
which decides, loads, and verifies that the assembly the loader returned
carries the identity the decision required. IsPermitted exposes the
decision half for tests that use names with no file on disk.
Also in the policy:
- Pin the culture of Microsoft.SqlServer.Types to neutral. It was left
server-controlled, so a Culture= in the reference could steer the bind.
- Distinguish an omitted public key token from an explicit
PublicKeyToken=null in allow list entries. AssemblyName represents the
first as null and the second as a zero-length array, and treating them
alike silently widened an entry written to pin an unsigned assembly
into one accepting any identity.
- Hold weak references to observed assemblies, and scope the
already-loaded tier to the driver's own AssemblyLoadContext. Strong
references would let a server force the map to be built with one denied
UDT and thereby pin unrelated plugin assemblies, preventing a
collectible context from unloading.
The SMI path gains the SqlUserDefinedTypeAttribute gate it was missing.
It applied the assembly policy but not the type check, so a bare or
permitted non-UDT name could still reach ValueUtilsSmi.NullUdtInstance,
which invokes the type's static Null member. It also now traces denials,
so the documented claim that every denial is observable holds on both
paths.
Tests:
- TryLoad_LoadedAssemblyWithWrongToken_IsRefused drives the real load
path and demonstrates the loader ignoring the requested token.
- Cover explicit PublicKeyToken=null, omitted token, and culture pinning.
- Assert the policy denial specifically rather than only that nothing was
loaded. Because the hostile assembly does not exist, "nothing loaded"
also passed against the vulnerable implementation.
- Add a positive control so the denial tests cannot pass by resolution
being broken outright.
- Assert the referenced-but-unloaded precondition instead of returning
quietly when it cannot be established.
Docs: correct the exception type to TypeLoadException, qualify the
code-execution ordering claim as measured on CoreCLR rather than stated
unconditionally, note that the attribute lookup is filtered and the
attribute sealed so it cannot itself run foreign code, and document that
trust is per process rather than per server.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
Thanks both, this was a genuinely useful review. Pushed a3d30a6 addressing it.
The main finding
The post-load identity issue was correct and was the most serious item raised. On .NET the loader ignores the public key token in an AssemblyName, so pinning the reference was decoration rather than enforcement. SqlAuthenticationProviderManager already documents this for the Azure extension assembly, and I had not carried the same reasoning across.
Rather than bolt a check onto each call site, the load moved inside the policy: TryResolve is replaced by TryLoad, which decides, loads, and verifies the returned identity as one operation, so a caller cannot obtain a decision without the enforcement attached. TryLoad_LoadedAssemblyWithWrongToken_IsRefused drives the real load path and fails without the check.
Alongside that: culture is now pinned, explicit PublicKeyToken=null is distinguished from an omitted token, and the loaded-assembly map holds weak references scoped to the driver's own AssemblyLoadContext.
Two findings from the suppressed list that deserved to be promoted
The SMI path was missing the attribute gate. It applied the assembly policy but not the SqlUserDefinedTypeAttribute check, and ValueUtilsSmi.NullUdtInstance invokes the type's static Null member. A bare name with no assembly part never reaches the assembly resolver at all, so that was a real hole on that path. Now gated, and denials there are traced, so the "every denial is traced" claim actually holds on both paths.
The documented exception type was wrong.ADP.TypeLoad yields TypeLoadException, not SqlException. Corrected.
On GetCustomAttributes running attribute constructors
@edwardneal - I took this seriously enough to measure it, because if true it would undercut the gate. It does not apply here, for two reasons:
SqlUserDefinedTypeAttribute is sealed, so a hostile assembly cannot subclass it to get its own constructor invoked through the filter.
We use the filtered overload, GetCustomAttributes(typeof(SqlUserDefinedTypeAttribute), false), which does not instantiate non-matching attributes.
Probe on net9.0, with a hostile assembly carrying a module initializer, a non-matching attribute whose constructor writes a file, and a type with a static constructor:
Step
module init
non-matching attr ctor
type cctor
Assembly.LoadFrom
no
no
no
GetType
no
no
no
GetCustomAttributes(typeof(T), false)
no
no
no
CustomAttributeData.GetCustomAttributes
no
no
no
InvokeMember("Null")
yes
no
yes
So CustomAttributeData and the filtered overload are equivalent here, and I left the existing call in place rather than change shared SqlUdtInfo code on a security fix. Worth revisiting if the attribute is ever unsealed or the lookup becomes unfiltered - I have noted that in the docs so the constraint is not accidentally lost.
Your point does hold for the unfiltered overload, which we do not use.
Your trust-ordering scenario
Your Assembly B walkthrough is right, and it is deliberate but was under-documented. The already-loaded tier makes the permitted set a property of the process, not of the connection: once anything is loaded, a UDT type inside it can be instantiated on any server's say-so. The attribute gate confines this to types written to be deserialized from SQL Server, but it is a genuine widening and now says so explicitly in the docs.
You are also right that building s_loadedAssemblies once has security properties and not just performance ones - rebuilding on demand would let a dependency of a permitted assembly inherit that permission. That is now stated as a property to preserve rather than left as an implementation detail.
ALC
Good catch. Fixed as described above: weak references so a collectible context can still unload, and the tier scoped to the driver's context rather than every assembly in the process.
Still open
I have replied separately on the SqlUserDefinedTypeRegistration question and on the UnregisteredUserDefinedType / SqlBulkCopy suggestions, since those are design decisions rather than fixes.
On SqlUserDefinedTypeRegistration and #4413 - @edwardneal, I think you are right about the destination, and I would like to get there. My hesitation is about sequencing rather than the design.
Why it is the better mechanism. Everything you list holds. Register<T>() with a static generic parameter forces the assembly to be loaded as a side effect of the call, which collapses the awkward part of this PR: the already-loaded tier exists precisely because the driver cannot otherwise tell "the application knows about this type" from "a server named something plausible". Registration states that intent directly. It is also per type rather than per assembly, which is strictly tighter - allow-listing Contoso.Udts today permits every UDT in it. And it removes the string parsing, the loaded-assembly map, the ALC scoping, and the weak-reference bookkeeping, all of which are machinery that exists only to approximate what registration would say outright. The two problems have the same shape: #4413 needs the app to name UDTs so the trimmer can see them, and this needs the app to name UDTs so the driver can trust them. One declaration serves both.
Why not in this PR. Three reasons, in order of weight:
This is on an MSRC schedule. New public API needs review, and if that review wants changes, the security fix is blocked behind an API discussion. I would rather not put those on the same critical path.
Feature | Trimming-compatible UDT deserialization #4413 is currently specified as a no-op at runtime - explicitly "doesn't do anything at runtime" and safe to call on Framework. Making it the trust boundary changes it into something with runtime state and security consequences. That is a real change to the proposal, not just an implementation of it, and it deserves to be argued on its own thread rather than settled inside a security review.
Registration alone cannot be the only mechanism without breaking every existing UDT application on upgrade, since none of them call it yet. There still has to be a default for unregistered types, which is what this PR defines.
What I would propose instead. Ship the deny-by-default boundary now, and treat the AppContext string as the escape hatch it is rather than the intended ergonomics - it is deliberately unpleasant to use. Then land #4413 and have Register<T>() feed the same policy as an additional permitted tier. The internal shape already accommodates this: TryDecide consults an ordered set of tiers, and registration slots in as one more without touching the enforcement or the post-load identity verification.
The interesting follow-up question is whether registration should eventually let an application turn off the already-loaded tier and run strictly on registered types. That is the per-server/per-type trust model that would close the Assembly B scenario from your other thread properly. I would not make it the default, but as an opt-in it is the strongest configuration available and worth designing towards.
If you would rather see the typed API in this PR, say so and I will add it - I just do not want to be the one who couples a security release to an API review without flagging the trade first.
Review raised SqlCommandBuilder and several SqlBulkCopy shapes as
possibly affected. Most are not, because they move UDT bytes without
interpreting them, but two do materialize the type and were missing from
the compatibility notes:
- SqlBulkCopy from a SqlDataReader between UDT columns, which reads each
value to test it for INullable.
- Table-valued parameters sourced from a SqlDataReader, which build SMI
metadata with throwing enabled.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
The repository testing guidance requires XML summaries for helper methods (including parameter/return documentation where applicable), but CreateUdtMetaData is undocumented, as are the new PolicyScope and AssemblyLoadRecorder helper members below. Add behavior-focused documentation for this test infrastructure before merging.
Second round of review feedback on the UDT assembly load policy.
Validate the whole identity, not just the token. The decision carried
only the required public key token, so a version or culture the policy
relied on was never confirmed against what the loader returned. Measured
on net9.0: requesting System.Private.CoreLib at version 109.0.0.0 returns
9.0.0.0, which the old check accepted. Binding redirects on .NET
Framework produce the same effect. Decision now carries the required
version and culture alongside the token, and each is enforced after the
load.
Stop dependencies inheriting the already-loaded permission. The
AssemblyLoad handler recorded every assembly that arrived after the map
was built, including those pulled in by the policy's own Assembly.Load
for a permitted assembly. A server could then name one of those
dependencies and have it permitted as "already loaded", which is exactly
the transitive trust the policy documents as denied. Policy-triggered
loads are now marked with a counted thread-static guard and excluded.
Cover the SMI path. The attribute gate and resolver added there had no
tests; all end-to-end coverage went through CheckGetExtendedUDTInfo.
Added denied-assembly, bare non-UDT, and valid-UDT cases against
SmiMetaData.Type.
Fix two defects in the identity test:
- It returned without asserting when the test assembly is strong-name
signed, so official signed builds silently lost the coverage. It now
uses a framework assembly and does not depend on signing at all.
- It asserted the .NET shape only. On .NET Framework the loader enforces
the strong name during binding and throws rather than returning the
wrong assembly, so it would have failed on net462. Both shapes are the
same refusal and both are now accepted, which also keeps the post-load
check from regressing unnoticed on .NET.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
This subscribes a handler from the driver's assembly to AppDomain.CurrentDomain.AssemblyLoad and never removes it. When SqlClient is loaded into a collectible AssemblyLoadContext (a scenario this policy explicitly supports), the AppDomain event retains the delegate/method from that context and can keep the context alive; weak references in the map do not prevent that root. Subscribe to the driver's load context or detach the handler during its Unloading event.
AppContext.GetData does not exist on .NET Framework, so the net462 leg
of the build failed with CS0117 while every .NET leg compiled fine.
AppDomain.CurrentDomain.GetData is the portable equivalent. It has been
present since .NET Framework 1.1, and on .NET it is implemented over the
same AppContext data store, so it reads values written by either
AppContext.SetData or AppDomain.SetData as well as runtimeconfig.json
configProperties. Verified all three on net9.0.
The documented way to set the allow list is unchanged: the docs already
show AppDomain.CurrentDomain.SetData, which works on both frameworks.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
Two further findings from review, both cases where a permission granted
to one assembly could be inherited by another.
Confirm the simple name after the load. Every basis for permitting a
load rests on the simple name: it is what the allow list is matched on
and what the built-in exemption recognizes. Nothing checked it once the
loader returned, and for a simple-name allow list entry no other
component was constrained either, so SatisfiesRequiredIdentity took its
all-null early-out and accepted whatever arrived. A custom
AssemblyResolve handler or AssemblyLoadContext resolver could answer
with an unrelated assembly and have it inherit the permission. Decision
now carries the matched name and it is always verified, so the early-out
is gone.
Snapshot the loaded set before any policy-triggered load. The provenance
guard added previously only suppressed the AssemblyLoad callback, which
left the first call unprotected: the map is built lazily, so a first
request permitted by the allow list loaded before the map existed, and
the dependencies that arrived with it were then captured by the later
snapshot as though the application had brought them in. A server naming
one of them was accepted as already-loaded. TryDecide now takes the
snapshot up front, which also attaches the handler early enough that
subsequent loads are attributed rather than absorbed.
Both tests were confirmed to fail with their respective fix reverted and
to pass with it applied.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c
This new test helper has no XML documentation, while the repository's testing guidance requires summaries for helper methods and parameter/return documentation where applicable. Add a behavior-focused summary plus <param> and <returns> so the metadata construction and its purpose are documented.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When the driver materializes a UDT value, the type's assembly-qualified name comes from the server. That name previously flowed into
Assembly.LoadandType.GetTypewithout the driver making any decision about it, so the set of assemblies a connection could pull into the process was effectively chosen by the server rather than by the application.This PR puts the application back in control with a deny-by-default policy, plus a validation gate on the resolved type.
Policy (
UdtAssemblyPolicy)A single enforcing behavior that permits exactly three things:
Microsoft.SqlServer.TypesEverything else is refused. Notably, an assembly that is only statically referenced by a loaded assembly is not permitted, because loading it is a genuinely new load, which is precisely the decision this keeps with the application.
Applications configure the allow list through an AppContext data element:
Each entry matches only on the components it specifies, so a simple name permits any version/culture/PKT while a fully-qualified name must match exactly.
Type validation
Independently of the assembly decision, a resolved type that is not annotated with
SqlUserDefinedTypeAttributeis now rejected before any of its code runs. This is the gate that actually prevents foreign code execution. I verified empirically on CoreCLR that neitherAssembly.Load, nor resolving a type, nor reading that type's custom attributes runs anything from the target assembly. A module initializer or static constructor runs on first real member access, which is whatGetUdtValuewould otherwise perform. So the attribute check sits in front of the only step that executes code.SmiMetaData.Typehad a second, latent sink for the same pattern; it is now routed through the same policy as defense in depth, even though all live callers passnulltoday.Escape hatch
Switch.Microsoft.Data.SqlClient.UseLegacyUdtAssemblyLoadrestores the previous behavior in full. It is intended as a temporary compatibility measure, not a supported configuration, and is documented as such.Backwards compatibility
This is a behavior change for applications using custom UDTs, and I did not find a way to avoid it without leaving the hole open. Flagging it prominently rather than burying it.
The built-in spatial types (
SqlGeography,SqlGeometry,SqlHierarchyId) are unaffected.An application is affected when the custom UDT's assembly is not already loaded at the moment the value is read. That happens whenever the driver materializes the value and the app never names the type in its own code: generic data access layers, micro-ORMs,
DataTable.Load, and schema discovery. In those cases the driver's ownAssembly.Loadwas the only thing pulling the assembly in. If the app statically names the UDT type, the JIT loads the assembly first and everything still works.Two different symptom shapes, and the second is the one worth reviewer attention:
reader[i],GetValue, UDT output parametersSqlExceptionnaming the assembly and the allow listGetFieldType,GetSchemaTable,GetColumnSchemanullfor the UDT column's type rather than throwingThe second row follows the pre-existing
fThrow: falsecontract on those paths, so I preserved it rather than changing unrelated behavior in a security fix. It is harder to diagnose, becauseGetFieldTypedoes not normally returnnulland a caller that dereferences the result sees an unrelatedNullReferenceException. To compensate, every denial is traced throughSqlClientEventSourceregardless of which path was taken, so event source tracing will always identify the assembly. I would welcome a second opinion on whether that trade is right, or whether these paths should throw despite the contract.The remedy in every case is to name the assembly on the allow list.
Documentation and localization
.github/instructions/features.instructions.mddocuments the switch, the permitted set, and a "Compatibility impact" section covering both symptom shapes.Two new resource strings (
SQLUDT_AssemblyNotAllowed,SQLUDT_TypeNotUserDefined) are added toStrings.resx. Localization will pick these up through the normal OneLocBuild flow after this merges.No public API surface changes, so no
ref/updates are needed.Issues
Tracked internally via the MSRC case and its linked ADO repair item. Intentionally not linking a public issue here while the case is under coordinated release.
Testing
Two new unit test files, 85 tests total across the policy and the switch:
UdtAssemblyPolicyTest.cscovers the policy in isolation: enforcement, the pinnedMicrosoft.SqlServer.Typesidentity (including rejecting a same-named assembly with the wrong PKT/version), deny-by-default, allow list matching at each level of qualification, and the already-loaded tier. IncludesResolve_LoadedAssembly_IgnoresServerSuppliedIdentity, which covers a bug found during self-review where a loaded assembly was matched on simple name but then loaded using the server's full reference, letting a server force a new load of a different version.UdtAssemblyLoadHardeningTest.csdrivesCheckGetExtendedUDTInfoend to end with hostile assembly-qualified names and asserts no load occurred. It also asserts the attribute gate rejects a non-UDT type without running its static constructor, reading the marker flag from a separate class so the assertion is meaningful.Both test classes join
AppContextSwitchTestCollectionso they serialize with the other AppContext-mutating tests.Also added coverage for a structural limitation worth knowing about: a bare type name with no assembly part never reaches the assembly resolver at all, so only the attribute gate stops it. Three regression tests pin that behavior.
Validation performed: clean build at 0 warnings under
TreatWarningsAsErrors; full unit suite 992 passed / 9 skipped / 3 failed, where the 3 failures are pre-existing macOS keychain issues inNativeColumnEncryptionKeyBaselineunrelated to this change.Gap: the
net462leg cannot be built on macOS, so it needs CI to validate. Related open question: I measured module-initializer timing only on CoreCLR. ECMA-335 permits a runtime to run module initializers at load time, and .NET Framework is unverified. A different result there would mean softening how the docs frame the ordering, but the design is safe either way since the attribute gate still runs before any member access.Guidelines